Allocate video surface object statically as a global #306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This seems like a good direction to me. |
This is a step towards making it possible to use the same SDL12_Surface for the video surface at all times, and only reallocating its underlying SDL 2 SDL_Surface. Signed-off-by: Simon McVittie <[email protected]>
This is a step towards making it possible to use the same SDL12_Surface for the video surface at all times, and only reallocating its underlying SDL 2 SDL_Surface. Signed-off-by: Simon McVittie <[email protected]>
The part before we create the SDL 1.2 surface (creating the SDL 2.0 surface) becomes CreateRGBSurface(), and the part after becomes Surface12SetMasks(). Signed-off-by: Simon McVittie <[email protected]>
The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result of functions that return a "borrowed" pointer to the video surface, namely SDL_SetVideoMode() and SDL_GetVideoSurface(). (See PerlGameDev/SDL#305) When we would previously have allocated or freed the video surface wrapper object, instead allocate or free its contents in-place. When checking whether the video surface exists, because we never destroy it, we must now also check whether its underlying SDL2 video surface exists. Resolves: libsdl-org#305 Signed-off-by: Simon McVittie <[email protected]>
|
Rebased on main and fixed a missing NULL check. Briefly tested with:
and seems to work as intended. |
|
I also contributed a fix for the incorrect free (PerlGameDev/SDL#306) which was applied to Debian's libsdl-perl package, but I haven't seen any sign of it being reviewed upstream. I also accidentally found PerlGameDev/SDL#307 while testing that change, and it hasn't had any response either, so PerlGameDev/SDL might be relatively dead. |
|
Okay, well let's merge this then. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #305, we have trouble with libsdl-perl making assumptions about the video surface never being freed or reallocated, and it being a no-op to "free" the video surface.
There is nothing that says a SDL 1.2 surface must always wrap the same SDL 2 surface, so we can maybe accommodate those assumptions by having the video surface be a special-cased global pointer that is never freed, and only reallocate its contents?
This seems to work with libsdl-perl's
core_video.ttest, but I haven't tested it extensively; consider it a proof-of-concept for now.Factor out Surface20to12InPlace
This is a step towards making it possible to use the same SDL12_Surface
for the video surface at all times, and only reallocating its underlying
SDL 2 SDL_Surface.
Factor out FreeSurfaceContents, and handle surface20 more defensively
This is a step towards making it possible to use the same SDL12_Surface
for the video surface at all times, and only reallocating its underlying
SDL 2 SDL_Surface.
Factor out enough of SDL_CreateRGBSurface to create surfaces in-place
The part before we create the SDL 1.2 surface (creating the SDL 2.0
surface) becomes CreateRGBSurface(), and the part after becomes
Surface12SetMasks().
Allocate video surface object statically as a global
The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result
of functions that return a "borrowed" pointer to the video surface,
namely SDL_SetVideoMode() and SDL_GetVideoSurface().
(See Tries to free previous results of SDL_SetVideoMode(), leading to use-after-free crash PerlGameDev/SDL#305)
When we would previously have allocated or freed the video surface
wrapper object, instead allocate or free its contents in-place.
When checking whether the video surface exists, because we never destroy
it, we must now also check whether its underlying SDL2 video surface
exists.
Resolves: makes SDL_perl regress when it (incorrectly) frees a former video surface #305